142 refactor solvedac client/service 역할 분리#143
Conversation
## 역할 분리 - SolvedAcClient: 순수 HTTP 호출만 담당 - searchProblems(query, sort, direction) 시그니처 변경 - hasUserSolvedProblem, getSolvedProblemsRaw 제거 - SolvedAcService: 비즈니스 로직 통합 - 쿼리 조합, 응답 가공, 결과 판단을 Service에서 처리 ## 정리 - TestController 삭제 (프로덕션에 있던 테스트용 컨트롤러) - fetchSolvedProblems, recommendTest 삭제 (dead code) - 주석 처리된 커넥션 풀 설정 코드 제거
## 패키지 구조 - api/ → client/ 폴더명 변경 (역할 명확화) ## SolvedAcService 개선 - 오버로드 메서드 제거 → 단일 메서드로 통합 - 예외 처리 일관성 추가 (try-catch) - null 체크 일관성 추가 - 기본 난이도 범위 변경: 골드(11-15) → 전체(1-30) - private 메서드 인라인화 (depth 3 → 2) - 로그 레벨 조정: 유저 못 찾음 warn → info ## ProblemService 개선 - 오버로드 메서드 정리
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note
|
| Cohort / File(s) | Summary |
|---|---|
인프라 클라이언트 추가/분리 src/main/java/com/ryu/studyhelper/infrastructure/solvedac/SolvedAcClient.java, src/main/java/com/ryu/studyhelper/infrastructure/solvedac/client/SolvedAcRestClient.java |
원격 호출(REST)과 비즈니스 쿼리/가공/예외 처리를 분리. SolvedAcClient는 recommendUnsolvedProblems, getUserInfo/getUserBio, hasUserSolvedProblem 구현 및 동적 쿼리 빌드 담당. |
서비스/필드 참조 교체 src/main/java/com/ryu/studyhelper/solvedac/SolvedAcService.java, src/main/java/com/ryu/studyhelper/problem/service/ProblemService.java, src/main/java/com/ryu/studyhelper/member/MemberService.java, src/main/java/com/ryu/studyhelper/member/verification/BojVerificationFacade.java |
기존 SolvedAcService/클라이언트 구조 재정비: 의존성 필드명과 타입을 인프라 클라이언트로 교체, recommend API 시그니처 통합 및 오버로드 정리, 상수 및 헬퍼 메서드 추가. |
DTO 패키지 이동 / 추가 src/main/java/com/ryu/studyhelper/infrastructure/solvedac/dto/* |
여러 DTO 패키지를 ...infrastructure.solvedac.dto로 이동. 새로운 SolvedAcUserBioResponse 추가. ProblemInfo, ProblemSearchResponse, SolvedAcTagInfo, SolvedAcUserResponse, SolvedAcVerificationResponse 패키지 경로 변경. |
제거된 레거시/테스트 코드 src/main/java/com/ryu/studyhelper/solvedac/api/SolvedAcClient.java, src/main/java/com/ryu/studyhelper/solvedac/TestController.java, src/main/java/com/ryu/studyhelper/solvedac/dto/BojVerificationDto.java |
기존 API 클라이언트(순수 HTTP 구현 겹침), 테스트용 컨트롤러, 중복 DTO 삭제. |
경로/임포트 정리(소비자 코드) src/main/java/com/ryu/studyhelper/problem/ProblemController.java, .../problem/dto/TeamProblemRecommendResponse.java, .../problem/service/ProblemSyncService.java, .../recommendation/RecommendationService.java |
ProblemInfo 등 DTO 임포트 경로를 infrastructure 패키지로 전환. |
테스트 갱신 src/test/java/com/ryu/studyhelper/member/MemberServiceTest.java |
테스트의 모킹/주입 대상을 SolvedAcService → SolvedAcClient로 교체. 테스트 로직은 동일. |
Sequence Diagram(s)
sequenceDiagram
actor Caller
participant Service as "ProblemService / MemberService"
participant Client as "SolvedAcClient (infra)"
participant Rest as "SolvedAcRestClient"
participant API as "solved.ac API"
Caller->>Service: recommend(handles,...)
Service->>Client: recommendUnsolvedProblems(handles,count,minLevel,maxLevel,tagKeys)
Client->>Client: buildRecommendQuery(handles,minLevel,maxLevel,tagKeys)
Client->>Rest: searchProblems(query, sort="random", direction="asc")
Rest->>API: HTTP GET /search/problem?query=...
API-->>Rest: ProblemSearchResponse JSON
Rest-->>Client: ProblemSearchResponse
Client->>Client: map to ProblemInfo + apply exclusions/limit
Client-->>Service: List<ProblemInfo>
Service-->>Caller: response
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
- 42 refactor&feat 문제 정보 스키마 변경알고리즘 태그 #118: 태그 기반 필터링 및 recommendUnsolvedProblems 시그니처 확장(핵심 변경사항과 동일한 영역).
- 52 feature 유저의 문제해결 인증 api #55: hasUserSolvedProblem 및 solved.ac 연동 API/클라이언트 변경 관련 중첩 변경이 있음.
- 24 feature 회원탈퇴 #73: MemberService 관련 의존성/필드 변경과 일부 중복되는 멤버 로직 수정이 있음.
Poem
🐰 패키지를 깔끔히 옮겼어요, 쿼리는 내가 짜고
REST는 조용히 요청만 하네, 헷갈림은 없어졌죠 🥕
DTO는 제자리를 찾아가고, 테스트는 가벼워졌어요
역할 나눈 오늘의 커밋, 당근 한 조각 축하해요 🎉
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 40.63% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | PR 제목은 변경사항의 핵심인 SolvedAc 클라이언트와 서비스의 역할 분리를 명확하게 요약하고 있습니다. |
| Linked Issues check | ✅ Passed | PR의 모든 변경사항이 #142 이슈의 Phase 1과 Phase 2 목표를 충족합니다. 패키지 이동, 컴포넌트 리네이밍, 역할 분리, 테스트 코드 정리 모두 체크리스트 항목과 일치합니다. |
| Out of Scope Changes check | ✅ Passed | 모든 변경사항이 #142에서 정의한 범위 내에 있습니다. 패키지 재구성, 컴포넌트 리네이밍, import 경로 업데이트, 테스트 코드 수정 등이 일관성 있게 진행되었습니다. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
142-refactor-solvedac-clientservice-역할-분리
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/ryu/studyhelper/problem/service/ProblemService.java (1)
75-80:⚠️ Potential issue | 🟡 Minorhandles와 emails 인덱스 동기화 위험성
Line 75-77에서
handles.get(i)와emails.get(i)를 동일 인덱스로 접근하는데, 두 리스트의 순서가 동일하다고 보장되지 않으면 잘못된 핸들이 이메일 제목에 사용될 수 있습니다.또한
System.out.println은 프로덕션 코드에서 로거로 대체해야 합니다.🛠️ 수정 제안
- for (int i=0; i<emails.size(); i++) { - String subject = "%s님, 오늘의 추천문제입니다!".formatted(handles.get(i)); - mailSendService.sendTxtEmail(new MailTxtSendDto(emails.get(i), subject, content)); - System.out.println("메일 전송 완료: " + emails.get(i)); - } + for (int i = 0; i < emails.size(); i++) { + String subject = "%s님, 오늘의 추천문제입니다!".formatted(handles.get(i)); + mailSendService.sendTxtEmail(new MailTxtSendDto(emails.get(i), subject, content)); + log.info("메일 전송 완료: {}", emails.get(i)); + }인덱스 동기화 문제는
teamMemberRepository에서 핸들과 이메일을 함께 조회하는 DTO를 반환하도록 개선하면 해결됩니다.
🧹 Nitpick comments (4)
src/main/java/com/ryu/studyhelper/solvedac/client/SolvedAcClient.java (3)
25-25: 파라미터명 네이밍 컨벤션 위반
ResponseType은 파라미터이므로 lowerCamelCase(responseType)를 사용해야 합니다.♻️ 수정 제안
- private <T> T get(String path, Map<String, String> params, Class<T> ResponseType) { + private <T> T get(String path, Map<String, String> params, Class<T> responseType) { return rest.get() .uri(b -> { var ub = b.path(path); if (params != null) params.forEach(ub::queryParam); return ub.build(); // 인코딩/결합은 RestClient에 맡김 }) .accept(MediaType.APPLICATION_JSON) .retrieve() - .body(ResponseType); + .body(responseType); }
16-21: RestClient를 외부에서 주입받도록 개선 검토현재 RestClient를 생성자 내부에서 직접 빌드하고 있어 테스트 시 모킹이 어렵습니다.
RestClient또는RestClient.Builder를 주입받는 방식을 고려해보세요.♻️ 수정 제안 (선택적)
`@Component` public class SolvedAcClient { private final RestClient rest; - public SolvedAcClient() { - this.rest = RestClient.builder() + public SolvedAcClient(RestClient.Builder restClientBuilder) { + this.rest = restClientBuilder .baseUrl("https://solved.ac/api/v3") .defaultHeader("User-Agent", "studyhelper/1.0") .build(); }
38-40: getUserInfo와 getUserBio 중복 API 호출두 메서드 모두
/user/show엔드포인트를 호출하지만 다른 DTO로 역직렬화합니다. 의도적인 설계라면 괜찮지만,SolvedAcUserResponse에bio필드가 이미 포함되어 있다면 하나의 메서드로 통합하는 것이 효율적입니다.Also applies to: 63-65
src/main/java/com/ryu/studyhelper/problem/service/ProblemService.java (1)
65-77: 주석 처리된 코드 정리 필요Line 65, 76의 주석 처리된 코드는 불필요하므로 제거하는 것이 좋습니다.
- SolvedAcService → SolvedAcClient (@service → @component) - SolvedAcClient → SolvedAcRestClient (client/ 하위 유지) - BojVerificationDto → SolvedAcUserBioResponse - SolvedAcClient에 getUserBio() 메서드 추가 (예외 처리 포함) - BojVerificationFacade: RestClient 직접 호출 → SolvedAcClient 경유 - MemberService 필드명 solvedacService → solvedAcClient 통일 - solvedac/ → infrastructure/solvedac/ 패키지 이동 - 전체 import 경로 업데이트
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/java/com/ryu/studyhelper/infrastructure/solvedac/client/SolvedAcRestClient.java`:
- Line 25: The parameter name ResponseType in the private method get(String
path, Map<String, String> params, Class<T> ResponseType) violates Java
lowerCamelCase conventions; rename the parameter to responseType across the
method signature and all usages within the get method and any callers
referencing that parameter name to maintain consistency (refer to the get method
declaration and any internal references to ResponseType).
In
`@src/main/java/com/ryu/studyhelper/infrastructure/solvedac/SolvedAcClient.java`:
- Around line 88-91: The query delimiter is inconsistent between
buildRecommendQuery (uses String.join("+", conditions)) and hasUserSolvedProblem
(builds "id:<problemId> s@<handle>" with a space); make them consistent by using
'+' as the delimiter everywhere: in hasUserSolvedProblem replace the
space-separated DSL fragment ("id:" + problemId + " s@" + handle) with a
'+'-separated fragment ("id:" + problemId + "+s@" + handle) so all query pieces
follow the same DSL and match buildRecommendQuery's use of conditions joined by
'+'.
🧹 Nitpick comments (6)
src/main/java/com/ryu/studyhelper/infrastructure/solvedac/client/SolvedAcRestClient.java (2)
16-21: Base URL 하드코딩 및 타임아웃 미설정.외부 API 호출 시 타임아웃이 설정되지 않으면, solved.ac 서버 응답 지연 시 스레드가 무한 대기할 수 있습니다. 또한 base URL은
application.yml등 외부 설정으로 관리하는 것이 환경별 유연성에 좋습니다.♻️ 타임아웃 및 설정 외부화 제안
+import org.springframework.beans.factory.annotation.Value; +import org.springframework.http.client.SimpleClientHttpRequestFactory; + `@Component` public class SolvedAcRestClient { private final RestClient rest; - public SolvedAcRestClient() { + public SolvedAcRestClient(`@Value`("${solvedac.api.base-url:https://solved.ac/api/v3}") String baseUrl) { + SimpleClientHttpRequestFactory factory = new SimpleClientHttpRequestFactory(); + factory.setConnectTimeout(5000); + factory.setReadTimeout(5000); this.rest = RestClient.builder() - .baseUrl("https://solved.ac/api/v3") + .baseUrl(baseUrl) + .requestFactory(factory) .defaultHeader("User-Agent", "studyhelper/1.0") .build(); }
38-65:getUserInfo와getUserBio가 동일한 엔드포인트(/user/show)를 호출합니다.두 메서드 모두
/user/show?handle=...을 호출하지만 서로 다른 DTO로 역직렬화합니다. 현재 설계상 의도된 것이라면 괜찮지만, 향후 하나의 호출로 통합하고 상위 레이어에서 필요한 필드를 추출하는 방식도 고려해 보세요.src/main/java/com/ryu/studyhelper/member/MemberService.java (1)
84-93:getUserInfo반환값 미사용 — 의도를 명확히 하는 것을 권장합니다.
solvedAcClient.getUserInfo(handle)호출의 반환값을 사용하지 않고 있습니다. 존재 여부 확인만이 목적이라면getUserBio대신 별도의verifyUserExists(handle)같은 메서드를 두거나,getUserInfo가 존재 여부 확인용임을 주석이나 네이밍으로 명시하면 가독성이 향상됩니다. 현재 동작에는 문제 없으나, 의도를 더 명확히 표현할 수 있습니다.src/main/java/com/ryu/studyhelper/infrastructure/solvedac/SolvedAcClient.java (2)
29-39:getUserInfo와getUserBio의 예외 처리 로직이 동일합니다.두 메서드의 try-catch 패턴이 동일하므로, 공통 헬퍼 메서드로 추출하면 중복을 줄일 수 있습니다. 현재 규모에서는 큰 문제가 아니므로 향후 메서드 추가 시 고려해도 좋습니다.
♻️ 예시: 공통 예외 처리 헬퍼 추출
+ private <T> T callWithExceptionHandling(String handle, String operation, + java.util.function.Function<String, T> apiCall) { + try { + return apiCall.apply(handle); + } catch (HttpClientErrorException.NotFound e) { + log.info("solved.ac user not found: {}", handle); + throw new CustomException(CustomResponseStatus.SOLVED_AC_USER_NOT_FOUND); + } catch (Exception e) { + log.error("Failed to {} from solved.ac: {}", operation, handle, e); + throw new CustomException(CustomResponseStatus.SOLVED_AC_API_ERROR); + } + }Also applies to: 100-113
49-69:recommendUnsolvedProblems의 catch-all이CustomException도 잡을 수 있습니다.현재
solvedAcRestClient가CustomException을 던지지 않으므로 실질적 문제는 아니지만, 방어적으로CustomException을 먼저 재throw하면 향후 내부 로직 변경 시 예외가 의도치 않게SOLVED_AC_API_ERROR로 감싸지는 것을 방지할 수 있습니다.🛡️ 방어적 예외 처리 제안
} catch (Exception e) { + if (e instanceof CustomException) throw (CustomException) e; log.error("Failed to recommend problems for handles: {}", handles, e); throw new CustomException(CustomResponseStatus.SOLVED_AC_API_ERROR); }src/main/java/com/ryu/studyhelper/problem/service/ProblemService.java (1)
75-79:System.out.println사용 — 이번 PR 범위는 아니지만 로거 전환을 권장합니다.Line 79의
System.out.println은 이번 리팩터링 변경 사항은 아니지만, 클래스에 이미@Slf4j등의 로깅 인프라가 있다면log.info()로 전환하는 것이 운영 환경에서의 관리에 유리합니다. 별도 이슈로 추적하는 것을 고려해 주세요.
관련 이슈
변경 내용
변경 유형
테스트
스크린샷 (UI 변경 시)
참고사항
Summary by CodeRabbit
릴리스 노트